Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Server-side implementation of incremental subscription changes #2030

Merged
merged 20 commits into from
Jan 2, 2025

Conversation

jsdt
Copy link
Contributor

@jsdt jsdt commented Dec 2, 2024

Description of Changes

This adds the new message types for being able to subscribe and unsubscribe to individual queries without affecting other subscriptions. Much of this was taken from #1997, but this version does not remove the existing way of subscribing to a set of queries. This renames that to legacy_ in many places, and we eventually want to get rid of it, but by making this an additive change we don't need to update all the clients at the same time.

This biggest changes are in the SubscriptionManager code. Previously we just stored two mappings:

  1. Query hash to the set of clients subscribing, and
  2. Table id to the set of related queries

Now we also store a mapping from client to the set of subscriptions for that client (which is useful for unsubscribing), and we keep track of both legacy subscriptions (which have a set of subscriptions per client), and the new subscriptions (which are identified by a (ClientId, RequestId) pair).

API and ABI breaking changes

This is additive, and only adds new enum types to existing messages, so this is not breaking. In the future we will want to make a breaking change to remove the legacy versions.

Expected complexity level and risk

  1. This will add some complexity to the client, but this is mostly just tweaking the existing logic.

Testing

This passes existing tests (which exercise the legacy versions), and there are a few basic tests for the new version.

More unit test coverage for incremental subscription changes would be good, but I will probably add that when I start adding client support for it.

Follow-up work

We still need:

  1. Client support for this.
  2. More testing
  3. Handling of module hot swapping. If a module is republished, and a change to indexes breaks any queries, we need to send errors to clients for those queries.

@jsdt jsdt marked this pull request as ready for review December 2, 2024 20:29
@jsdt jsdt requested review from Centril and gefjon as code owners December 2, 2024 20:29
crates/client-api-messages/src/websocket.rs Outdated Show resolved Hide resolved
crates/client-api-messages/src/websocket.rs Outdated Show resolved Hide resolved
crates/client-api-messages/src/websocket.rs Outdated Show resolved Hide resolved
crates/client-api-messages/src/websocket.rs Outdated Show resolved Hide resolved
crates/client-api-messages/src/websocket.rs Outdated Show resolved Hide resolved
crates/core/src/error.rs Outdated Show resolved Hide resolved
@gefjon gefjon assigned jsdt and unassigned gefjon Dec 3, 2024
crates/client-api-messages/src/websocket.rs Show resolved Hide resolved
crates/core/src/client/message_handlers.rs Show resolved Hide resolved
crates/core/src/client/messages.rs Show resolved Hide resolved
crates/core/src/client/messages.rs Show resolved Hide resolved
@bfops bfops added the release-any To be landed in any release window label Dec 30, 2024
@jsdt jsdt requested a review from gefjon December 31, 2024 16:25
@jsdt jsdt requested a review from Centril December 31, 2024 16:25
@jsdt jsdt assigned Centril and gefjon and unassigned jsdt Dec 31, 2024
Copy link
Contributor

@gefjon gefjon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm interested in your responses to the few comments I haven't resolved, but I'd be happy to merge this as-is.

@gefjon gefjon dismissed Centril’s stale review December 31, 2024 19:18

@jsdt has addressed comments. @Centril is out of office, but we'd like to merge this before it grows stale.

@jsdt jsdt added this pull request to the merge queue Jan 2, 2025
Merged via the queue into master with commit e9e287b Jan 2, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-any To be landed in any release window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Subscriptions: New WebSocket API and server side handling
4 participants